-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
parametrize Bidiagonal on wrapped vector type #22925
Conversation
@@ -97,29 +97,30 @@ srand(1) | |||
end | |||
|
|||
@testset "triu and tril" begin | |||
bidiagcopy(dv, ev, uplo) = Bidiagonal(copy(dv), copy(ev), uplo) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hm, so was this previously writing zeros through to these?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No but previously the constructor always made a copy of the vectors, ref e0d75d7#commitcomment-22968553
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
were Tridiagonal or SymTridiagonal behaving like that too, or was Bidiagonal the only case of it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
e0d75d7 only changed the behaviour of Bidiagonal
. The other ones still have constructors that don't copy: Diagonal, Tridiagonal and SymTridiagonal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
generally lgtm! :)
Restarting Travis, I saved the logs if anyone is interested, but:
and some (spurious?) distributed failure on macOS. |
6177b3b
to
1a13b38
Compare
Added some more constructors which promotes more properly, and tests. |
base/linalg/bidiag.jl
Outdated
end | ||
|
||
function Bidiagonal(dv::AbstractVector{Td}, ev::AbstractVector{Te}, uplo::Symbol) where {Td,Te} | ||
T = promote_type(Td,Te) | ||
function Bidiagonal(dv::AbstractVector{T}, ev::AbstractVector{T}, uplo::Symbol) where T | ||
Bidiagonal(convert(Vector{T}, dv), convert(Vector{T}, ev), uplo) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isn't this conversion to Vector what you're trying to make unnecessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just a fallback for cases like Bidiagonal(::Vector{T}, ::GenericVector{T})
, but perhaps the constructor on line 62 should be generalized to promote to the same container type as well, and not just promote the element type? That would make this constructor obsolete.
Currently the promotion is as follows:
Bidiagonal(::Vector{T}, ::GenericVector{S}) ->
Bidiagonal(::Vector{R}, ::GenericVector{R}) ->
Bidiagonal(::Vector{R}, ::Vector{R})
The problem with generalizing the method above was this:
julia> A = rand(3); B = GenericArray(rand(3)); a, b = promote(A,B);
julia> typeof(a)
Array{Float64,1}
julia> typeof(b)
Base.Test.GenericArray{Float64,1}
i.e. they don't promote to the same container type. Hence the current scheme.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking about this a bit more, perhaps we should not promote anything in the constructor here, and instead let the user promote to appropriate types. In that case this:
function Bidiagonal(dv::V, ev::V, uplo::Symbol) where {T,V<:AbstractVector{T}}
Bidiagonal{T}(dv, ev, char_uplo(uplo))
end
would be the only constructor. The good thing about this is that we don't try to "figure out" what the user wants; instead the user have to be explicit about this. The other good thing with this method is that the vectors in the Bidiagonal
will always alias the vectors sent to the constructor. This would then be consistent with the new Diagonal
type from #22718 . As this PR currently stands, the vectors will sometimes alias and sometimes not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We would also have to deprecate methods that did convert, but I still like this better. It is probably not very often you supply two different vector types anyway; and it would be consistent that the vectors always alias, so I will update the PR like that later, unless other people think that’s a bad idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or promote in a way consistent with what concatenation would do, since this is sort of like a diagonal concatenation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the aliasing predictability benefits of not promoting at all and making the user pick could also have advantages though - as long as the old methods were deprecated, and the restriction is documented, I think that would also be fine and not trip up too many people
1a13b38
to
b2d4167
Compare
Alright, updated the PR. |
b2d4167
to
ff3d5a0
Compare
ff3d5a0
to
aa3ee45
Compare
base/deprecated.jl
Outdated
# PR #22925 | ||
# also uncomment constructor tests in test/linalg/bidiag.jl | ||
function Bidiagonal(dv::AbstractVector{T}, ev::AbstractVector{S}, uplo::Symbol) where {T,S} | ||
depwarn(string("Bidiagonal(dv::AbstractVector{T}, ev::AbstractVector{S}, uplo::Symbol) ", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add a bullet for this deprecation in NEWS
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, updated.
1 ⋅ ⋅ ⋅ | ||
2 2 ⋅ ⋅ | ||
⋅ 3 3 ⋅ | ||
⋅ ⋅ 4 4 | ||
``` | ||
""" | ||
Bidiagonal(A::AbstractMatrix, uplo::Symbol) = Bidiagonal(diag(A), diag(A, uplo == :U ? 1 : -1), uplo) | ||
function Bidiagonal(A::AbstractMatrix, uplo::Symbol) | ||
Bidiagonal(diag(A, 0), diag(A, uplo == :U ? 1 : -1), uplo) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't actually realize diag(A, k)
would already give a SparseVector
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, as long as the two arg form is inferable this should be "more correct" since diag(A)
in some cases give something else.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
which isn't very consistent with it acting like a default argument, but that's a discussion for a different issue
Added NEWS for the deprecations. Would like to merge this first and then fix up #23035 afterwards. |
NEWS.md
Outdated
@@ -147,6 +147,9 @@ Library improvements | |||
allowing `Diagonal` and `Bidiagonal` matrices with arbitrary | |||
`AbstractVector`s ([#22718], [#22925]). | |||
|
|||
* `Bidiagonal` constructors that automatically converted the input vectors to | |||
the same type are deprecated in favor of explicit conversion ([#22925]). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking this would go in the Deprecated or removed
section since the library functionality part of the change is mentioned earlier
I'd also add to the docstring what restrictions on the two inputs there are - in |
deprecate Bidiagonal constructor that automatically convert the vectors
8c061c9
to
7993842
Compare
Updated the docstring for the constructor. There are some |
I'm not seeing a docstring change? |
I only added the constraint to the signature, you want me to mention it in the text too? |
no, I just missed that, probably good enough |
Your benchmark job has completed - no performance regressions were detected. A full report can be found here. cc @ararslan |
win32 appveyor failure in distributed is a bit alarming, but highly doubt it's related |
It happens from time to time |
Good lord 😠. Getting our CI rock solid has got to be a top priority post 1.0 if not sooner. |
The randomly occurring distributed failure is a segfault . Stack shows gf.c, but then the corruption could have happened elsewhere too. The distributed module does not have any Long term CI plans should include a valgrind / memcheck run if possible. |
I'm not complaining about this particular issue really, just the general flakiness of our CI lately. A bunch of it is that Travis keeps giving us less resources so now many runs just time out. |
Same as #22718 but for
Bidiagonal
.